fix(management): refresh user name/email from ID-token claims on login#6671
fix(management): refresh user name/email from ID-token claims on login#6671Amilliox wants to merge 2 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates the login flow to refresh stored user Name and Email from non-empty token claims when they differ, persisting the change via ChangesLogin-time user profile sync
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
On subsequent logins, GetUserFromUserAuth only updated last_login but never refreshed name/email from the ID-token claims. Users created by older versions keep empty name/email forever, and those running with IdpManagerConfig.ManagerType: "none" also see blank fields. This change reads name/email from the login ID-token (already available in userAuth) and updates the stored record if they differ — mirroring the existing JWT group sync (JWTGroupsClaimName) pattern. No extra IdP API call, no background job, no SCIM, no license needed. Fixes netbirdio#6652 Related netbirdio#2073
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/user.go (1)
232-248: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid full-row user saves here.
GetUserFromUserAuthreads the user without a lock, then callsSaveUser, which persists the whole record. That can overwrite unrelated concurrent changes on the same user row. Use a scoped update forname/SaveUserLastLogin, instead of saving the full user object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/user.go` around lines 232 - 248, The user refresh logic in GetUserFromUserAuth is doing a full SaveUser after updating only name/email, which can overwrite unrelated concurrent changes. Replace that persistence path with a scoped store update for just the name/email fields, following the pattern used by SaveUserLastLogin or a similar narrow update helper, and keep the existing GetUserByUserID and logging flow intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@management/server/user.go`:
- Around line 245-247: The user update flow in user.go leaves the in-memory User
mutated even when am.Store.SaveUser fails, so callers can receive Name/Email
values that were never persisted. In the save path around SaveUser, capture the
original values before mutating and restore them if the save returns an error,
or otherwise return the pre-update user state on failure. Keep the fix localized
to the update logic that logs “failed to update user name/email” and returns the
user from the same method.
---
Outside diff comments:
In `@management/server/user.go`:
- Around line 232-248: The user refresh logic in GetUserFromUserAuth is doing a
full SaveUser after updating only name/email, which can overwrite unrelated
concurrent changes. Replace that persistence path with a scoped store update for
just the name/email fields, following the pattern used by SaveUserLastLogin or a
similar narrow update helper, and keep the existing GetUserByUserID and logging
flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2618edd-894a-4876-b8e8-b348c135e72c
📒 Files selected for processing (1)
management/server/user.go
If SaveUser fails after mutating user.Name/user.Email, restore the original values so callers do not see unsaved changes.
|
2 participants
Problem
On subsequent logins,
GetUserFromUserAuthonly updateslast_loginbut never refreshesname/emailfrom the ID-token claims. Users created by older versions keep empty name/email forever, and those running withIdpManagerConfig.ManagerType: "none"also see blank fields.Fix
After retrieving the user, check if
userAuth.NameoruserAuth.Emaildiffer from stored values (and are non-empty). If so, update and save — mirroring the existing JWT group sync (JWTGroupsClaimName) pattern.Scope
management/server/user.goFixes #6652
Related #2073
Summary by CodeRabbit